Recourse has to work with Recoursees that are not in the contact book #672

zupzup · 2025-10-06T13:52:21Z

User description

📝 Description

Recoursees don't have to be in the contact book anymore
Disable useless number-of-arguments clippy rule
Fix flaky test (the issue was with two now calls and a non-failing call on the function under test

Relates to #422

✅ Checklist

Please ensure the following tasks are completed before requesting a review:

My code adheres to the coding guidelines of this project.
I have run cargo fmt.
I have run cargo clippy.
I have added or updated tests (if applicable).
All CI/CD steps were successful.
I have updated the documentation (if applicable).
I have checked that there are no console errors or warnings.
I have verified that the application builds without errors.
I've described the changes made to the API. (modification, addition, deletion).

🚀 Changes Made

See above.

💡 How to Test

Please provide clear instructions on how reviewers can test your changes:

cargo test
do recourse against a recoursee that's not in the contact book - it needs to work

🤝 Related Issues

List any related issues, pull requests, or discussions:

For recourse we must be able to do action even without contact in book #422

📋 Review Guidelines

Please focus on the following while reviewing:

Does the code follow the repository's contribution guidelines?
Are there any potential bugs or performance issues?
Are there any typos or grammatical errors in the code or comments?

PR Type

Enhancement

Description

Enable recourse functionality for recoursees not in contact book
Add NostrContactStore integration to BillService
Update recourse validation to use past endorsees
Fix type conversion for PastEndorsee structure

Diagram Walkthrough

flowchart LR
  A["NostrContactStore"] --> B["BillService"]
  B --> C["Recourse Request"]
  C --> D["Past Endorsees Validation"]
  D --> E["Nostr Contact Lookup"]
  E --> F["Recourse Execution"]

File Walkthrough

Relevant files

Enhancement

service.rs `Add NostrContactStore integration to BillService` crates/bcr-ebill-api/src/service/bill_service/service.rs Add `NostrContactStoreApi` dependency to BillService Update constructor to accept nostr contact store parameter Integrate nostr contact lookup in notification logic	+8/-0
bill.rs `Update recourse request validation logic` crates/bcr-ebill-wasm/src/api/bill.rs Replace contact service lookup with nostr contact lookup Add past endorsees validation for recoursee verification Update public data creation using past endorsees and nostr contacts	+24/-12
bill.rs `Add type conversion for BillIdentParticipant` crates/bcr-ebill-wasm/src/data/bill.rs Add From trait implementation for BillIdentParticipant to LightBillIdentParticipantWeb conversion	+10/-0

Tests

test_utils.rs `Update test utilities for NostrContactStore` crates/bcr-ebill-api/src/service/bill_service/test_utils.rs Add MockNostrContactStore to test context Update service constructor calls with nostr contact store Configure mock expectations for nostr contact lookups	+10/-4
mod.rs `Update test mock expectations` crates/bcr-ebill-api/src/service/bill_service/mod.rs Replace identity block population expectation with identity chain events expectation in test	+3/-2

Bug fix

mod.rs `Fix PastEndorsee participant type` crates/bcr-ebill-core/src/bill/mod.rs Change PastEndorsee field type from LightBillIdentParticipant to BillIdentParticipant	+1/-1

Configuration changes

context.rs `Update context with NostrContactStore` crates/bcr-ebill-wasm/src/context.rs Update BillService constructor call to include nostr_contact_store parameter	+1/-0
clippy.toml `Increase clippy arguments threshold` clippy.toml Increase too-many-arguments-threshold from 14 to 200	+1/-1

Copilot

Pull Request Overview

This PR enables recourse functionality to work with recoursees that are not present in the contact book by fetching past endorsees to validate and obtain recoursee data, and integrating nostr contact store for relay information.

Modified recourse logic to use past endorsees instead of requiring contacts in the contact book
Added nostr contact store integration to BillService for relay information
Updated data structures to support full participant data in past endorsees

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/bcr-ebill-wasm/src/data/bill.rs	Added conversion from BillIdentParticipant to LightBillIdentParticipantWeb
crates/bcr-ebill-wasm/src/context.rs	Added nostr contact store to BillService initialization
crates/bcr-ebill-wasm/src/api/bill.rs	Modified recourse logic to use past endorsees and nostr contacts
crates/bcr-ebill-core/src/bill/mod.rs	Changed PastEndorsee field type to BillIdentParticipant
crates/bcr-ebill-api/src/service/bill_service/test_utils.rs	Added MockNostrContactStore to test setup
crates/bcr-ebill-api/src/service/bill_service/service.rs	Added nostr contact store integration to BillService
crates/bcr-ebill-api/src/service/bill_service/mod.rs	Updated test expectations
clippy.toml	Increased too-many-arguments threshold

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

clippy.toml

qodo-merge-pro · 2025-10-06T13:52:57Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Lax complexity limit

Description: Raising the too-many-arguments threshold to 200 can mask overly complex constructors or
functions, potentially increasing attack surface due to reduced scrutiny on parameter
validation and misuse; consider keeping a stricter threshold to maintain security-oriented
code hygiene.
clippy.toml [1-1]

Referred Code

too-many-arguments-threshold=200

Ticket Compliance

🟡

🎫 #422

🟢	Allow recourse actions to be executed even when the recoursee is not in the contact book.
	Validate that the recoursee is a past endorsee of the bill when requesting recourse.
	Integrate Nostr contact lookup to obtain relays for recourse notifications/delivery.
	Update services and tests to support the new Nostr contact store and past endorsee validation flow.
⚪	Manually verify end-to-end that recourse works in a real environment when the recoursee is not in the contact book, including Nostr relay delivery and notifications.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

⚪

No custom compliance provided

Follow the guide to enable custom compliance check.

Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-merge-pro · 2025-10-06T13:54:59Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Refactor service constructors to improve maintainability The `BillService` constructor has too many arguments (15), which was addressed by increasing a clippy lint threshold. It is suggested to refactor this by grouping the dependencies into a dedicated struct to improve code maintainability. Examples: crates/bcr-ebill-api/src/service/bill_service/service.rs [85-118] pub fn new( store: Arc<dyn BillStoreApi>, blockchain_store: Arc<dyn BillChainStoreApi>, identity_store: Arc<dyn IdentityStoreApi>, file_upload_store: Arc<dyn FileUploadStoreApi>, file_upload_client: Arc<dyn FileStorageClientApi>, bitcoin_client: Arc<dyn BitcoinClientApi>, notification_service: Arc<dyn NotificationServiceApi>, identity_blockchain_store: Arc<dyn IdentityChainStoreApi>, company_blockchain_store: Arc<dyn CompanyChainStoreApi>, ... (clipped 24 lines) clippy.toml [1] too-many-arguments-threshold=200 Solution Walkthrough: Before: // in crates/bcr-ebill-api/src/service/bill_service/service.rs impl BillService { pub fn new( store: Arc<dyn BillStoreApi>, blockchain_store: Arc<dyn BillChainStoreApi>, identity_store: Arc<dyn IdentityStoreApi>, file_upload_store: Arc<dyn FileUploadStoreApi>, file_upload_client: Arc<dyn FileStorageClientApi>, bitcoin_client: Arc<dyn BitcoinClientApi>, notification_service: Arc<dyn NotificationServiceApi>, identity_blockchain_store: Arc<dyn IdentityChainStoreApi>, company_blockchain_store: Arc<dyn CompanyChainStoreApi>, contact_store: Arc<dyn ContactStoreApi>, company_store: Arc<dyn CompanyStoreApi>, mint_store: Arc<dyn MintStoreApi>, mint_client: Arc<dyn MintClientApi>, court_client: Arc<dyn CourtClientApi>, nostr_contact_store: Arc<dyn NostrContactStoreApi>, ) -> Self { ... } } After: // in crates/bcr-ebill-api/src/service/bill_service/service.rs pub struct BillServiceDependencies { pub store: Arc<dyn BillStoreApi>, pub blockchain_store: Arc<dyn BillChainStoreApi>, pub identity_store: Arc<dyn IdentityStoreApi>, // ... other dependencies pub nostr_contact_store: Arc<dyn NostrContactStoreApi>, } impl BillService { pub fn new(deps: BillServiceDependencies) -> Self { Self { store: deps.store, blockchain_store: deps.blockchain_store, // ... nostr_contact_store: deps.nostr_contact_store, } } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a design smell (a constructor with 15 arguments) that the PR exacerbates, and it points out that the fix of increasing the `too-many-arguments-threshold` in `clippy.toml` is a workaround, not a solution.	Medium
General	Construct new participant data instead of mutating Instead of mutating a cloned `past_endorsee`, construct a new `BillIdentParticipant` by combining its historical data with the up-to-date `nostr_relays` from the `nostr_contact`. crates/bcr-ebill-wasm/src/api/bill.rs [943-955] // create public recourse data from past endorsees and our nostr contacts -let mut public_data_recoursee = match past_endorsees +let public_data_recoursee = match past_endorsees .iter() .find(\|pe\| &pe.pay_to_the_order_of.node_id == recoursee_node_id) { - Some(found_pe) => found_pe.pay_to_the_order_of.clone(), + Some(found_pe) => { + let pe_data = &found_pe.pay_to_the_order_of; + BillIdentParticipant { + t: pe_data.t, + node_id: pe_data.node_id.clone(), + name: pe_data.name.clone(), + postal_address: pe_data.postal_address.clone(), + email: pe_data.email.clone(), + nostr_relays: nostr_contact.relays, + } + } None => { return Err( BillServiceError::Validation(ValidationError::RecourseeNotPastHolder).into(), ); } }; -public_data_recoursee.nostr_relays = nostr_contact.relays; Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes constructing a new `BillIdentParticipant` instead of mutating a cloned one, which is a cleaner and more robust pattern for combining historical and current data.	Medium
General	Distinguish between not found and error Separate the error handling for a 'not found' contact from a database lookup failure. Log the specific error for database issues and return a more appropriate internal error. crates/bcr-ebill-wasm/src/api/bill.rs [925-935] // we fetch the nostr contact first to know where we have to send let nostr_contact = match get_ctx() .contact_service .get_nostr_contact_by_node_id(recoursee_node_id) .await { Ok(Some(nc)) => nc, - Ok(None) \| Err(_) => { + Ok(None) => { return Err(BillServiceError::RecourseeNotInContacts.into()); + } + Err(e) => { + error!("Failed to get nostr contact by node id: {}", e); + return Err(Error::Internal("Database error".to_string()).into()); } }; Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly improves error handling by distinguishing between a 'not found' case and a database error, which is crucial for logging and debugging.	Low
Update

zupzup · 2025-10-06T13:56:26Z

clippy.toml

@@ -1 +1 @@
-too-many-arguments-threshold=14
+too-many-arguments-threshold=200


I have zero patience for dogmatic nonsense such as too-many-arguments-for-a-function limits. 😅 This basically disables it globally.

until you get a warning at 200

codecov · 2025-10-06T14:07:36Z

Codecov Report

❌ Patch coverage is 24.24242% with 25 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/bcr-ebill-wasm/src/api/bill.rs	0.00%	15 Missing ⚠️
crates/bcr-ebill-wasm/src/data/bill.rs	0.00%	7 Missing ⚠️
.../bcr-ebill-api/src/service/bill_service/service.rs	60.00%	2 Missing ⚠️
crates/bcr-ebill-wasm/src/context.rs	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

zupzup requested review from tompro, mtbitcr, codingpeanut157, stefanbitcr and Copilot October 6, 2025 13:52

zupzup self-assigned this Oct 6, 2025

Copilot AI reviewed Oct 6, 2025

View reviewed changes

clippy.toml Show resolved Hide resolved

qodo-merge-pro bot added the Review effort 3/5 label Oct 6, 2025

Recourse has to work with Recoursees that are not in the contact book

258f3e9

zupzup force-pushed the 422-recourse-without-recoursee-in-contact-book branch from 0d42ed5 to 258f3e9 Compare October 6, 2025 13:54

zupzup commented Oct 6, 2025

View reviewed changes

stefanbitcr approved these changes Oct 6, 2025

View reviewed changes

properly fix test

5277c4f

mtbitcr approved these changes Oct 7, 2025

View reviewed changes

zupzup merged commit 7a47492 into master Oct 7, 2025
6 of 7 checks passed

zupzup deleted the 422-recourse-without-recoursee-in-contact-book branch October 7, 2025 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Recourse has to work with Recoursees that are not in the contact book #672

Recourse has to work with Recoursees that are not in the contact book #672

zupzup commented Oct 6, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

qodo-merge-pro bot commented Oct 6, 2025 •

edited

Loading

Uh oh!

qodo-merge-pro bot commented Oct 6, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

zupzup Oct 6, 2025

Uh oh!

stefanbitcr Oct 6, 2025

Uh oh!

codecov bot commented Oct 6, 2025

Uh oh!

Uh oh!

Uh oh!

		@@ -1 +1 @@
		too-many-arguments-threshold=14
		too-many-arguments-threshold=200

Recourse has to work with Recoursees that are not in the contact book #672

Recourse has to work with Recoursees that are not in the contact book #672

Conversation

zupzup commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

📝 Description

✅ Checklist

🚀 Changes Made

💡 How to Test

🤝 Related Issues

📋 Review Guidelines

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

qodo-merge-pro bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-merge-pro bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

zupzup Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

stefanbitcr Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 6, 2025

Codecov Report

Uh oh!

Uh oh!

Uh oh!

zupzup commented Oct 6, 2025 •

edited

Loading

qodo-merge-pro bot commented Oct 6, 2025 •

edited

Loading

qodo-merge-pro bot commented Oct 6, 2025 •

edited

Loading